test(connector-corda): fix flaky corda-v5-flow.test.ts #3322
Conversation
Fixes hyperledger-cacti#2978 Signed-off-by: adrianbatuto <[email protected]>
|
Depends on #3241 Please merge only after #3241has been merged as well. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const response = await fetch(`${req.baseUrl}flow/${holdingIDShortHash}`, { | ||
| method: `POST`, | ||
| headers, | ||
| body: cordaReqBuff, | ||
| agent, | ||
| }); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
There was a problem hiding this comment.
@petermetz the URL's are the input corresponding to the REST api calls (which will be a variable), so I think its a false positive CodeQL review point
There was a problem hiding this comment.
@jagpreetsinghsasan If I send in a request with baseUrl being set to http://evil.website.with.malware.example.com then we'll go to that link and bad things could happen.
Is there any reason why we can't pass in the hostname of the Corda node(s) in the constructor of the connector plugin? That would make it less flexible but then we only have to worry about a malicious system administrator who deployed the system with a bad configuration (an acceptable risk because if the person deploying the software is malicious, all is lost anyway)
| const response = await fetch(`${req.baseUrl}cpi`, { | ||
| method: `GET`, | ||
| headers: headers, | ||
| agent, | ||
| }); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
There was a problem hiding this comment.
The URL's are the input corresponding to the REST api calls (which will be a variable), so I think its a false positive CodeQL review point
| const response = await fetch( | ||
| `${req.baseUrl}flow/${holdingIDShortHash}/${clientRequestId}`, | ||
| { | ||
| method: `GET`, | ||
| headers: headers, | ||
| agent, | ||
| }, | ||
| ); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
There was a problem hiding this comment.
The URL's are the input corresponding to the REST api calls (which will be a variable), so I think its a false positive CodeQL review point
| const response = await fetch(`${req.baseUrl}flow/${holdingIDShortHash}`, { | ||
| method: `GET`, | ||
| headers: headers, | ||
| agent, | ||
| }); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
There was a problem hiding this comment.
The URL's are the input corresponding to the REST api calls (which will be a variable), so I think its a false positive CodeQL review point
| get: async () => ({ | ||
| isProtected: true, | ||
| requiredRoles: [], | ||
| httpsAgent: new https.Agent({ rejectUnauthorized: false }), |
Check failure
Code scanning / CodeQL
Disabling certificate validation High
There was a problem hiding this comment.
@adrianbatuto this piece of code is no longer used, can be removed safely
@adrianbatuto Please put the declaration in the commit message and the PR description otherwise the bot doesn't see it. |
Fixes hyperledger-cacti#3293 Signed-off-by: adrianbatuto <[email protected]>
adrianbatuto
force-pushed
the
adrianbatuto/issue3293
branch
from
3e1660d
to
bf0f92a
Compare
adrianbatuto
requested review from
petermetz,
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna,
sandeepnRES and
outSH
as code owners
There was a problem hiding this comment.
@adrianbatuto Please see above, same request as before.
One more in addition though: The security issues highlighted by CodeQL seem like true positives, could you please address those?
One more request: Please explain in detail why do you need the http agent property in the authorization config for the endpoint (in the commit message and the PR description)
|
This PR/issue depends on:
|
|
Closing this PR as the changes here have been included in #3241. |
Commit to be reviewed
test(connector-corda): fix flaky corda-v5-flow.test.ts
Fixes #3293
Depends on #2978
Pull Request Requirements
upstream/mainbranch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-sflag when usinggit commitcommand. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.